Skip to content

fix: make repair tests portable on Windows - #376

Merged
RomneyDa merged 7 commits into
openclaw:mainfrom
brokemac79:codex/windows-test-portability
Jul 1, 2026
Merged

fix: make repair tests portable on Windows#376
RomneyDa merged 7 commits into
openclaw:mainfrom
brokemac79:codex/windows-test-portability

Conversation

@brokemac79

@brokemac79 brokemac79 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

ClawSweeper's repair validation path and several test fixtures assumed POSIX command lookup, LF-only source reads, executable fake gh shims, and /-only path containment. That made Windows local validation fail even when the runtime behavior being tested was otherwise portable.

Why This Change Was Made

This updates repair command execution and GitHub CLI helpers to support explicit tool shims on Windows, makes .cmd/.bat validation commands run through cmd.exe with double-escaped batch arguments, fixes state-root publish containment with platform-native path checks, and normalizes source-fixture reads where assertions inspect tracked files. The test fixtures now use explicit Node-backed fake tools instead of depending on extensionless executables being discoverable through PATH.

The follow-up quoting hardening replaces regex-based Windows argument quoting with an explicit scanner that doubles trailing backslashes and backslashes before embedded quotes before applying the existing cmd.exe metacharacter escaping.

User Impact

Maintainers can run the ClawSweeper repair and unit suites from Windows without patching local tool shims or line endings. The state publish path also no longer rejects valid state-root paths on Windows, and Windows batch validation launchers preserve spaces, metacharacters, embedded quotes, and trailing backslashes in arguments.

Evidence

Head proofed: 3f76fb935334e82131ce043c360bcca9903895f9.

Exact-head note: the top commit is the session-only empty ClawSweeper re-review trigger. git diff --exit-code HEAD~1 HEAD --stat produced no tree diff, so the proofed code tree is the PR head tree. The code fix commit is c9b398bd2071b958bf8d8d2f48006f2c2d5bdb0c.

  • pnpm install --frozen-lockfile passed earlier on this branch; dependencies did not change in the follow-up.
  • pnpm exec oxfmt --write src/repair/command-runner.ts test/repair/command-runner.test.ts passed.
  • node --test test\repair\command-runner.test.ts passed on Windows after the quoting hardening: 4 tests, 4 pass.
  • A real .cmd launcher probe round-tripped arguments containing embedded quotes, trailing backslashes, double trailing backslashes, spaces plus trailing backslashes, ^, %PATH%, and ] with equal: true.
  • pnpm run check passed after the quoting hardening, including check:active-surface, check:limits, build:all, lint, test:unit, test:repair, changed/full coverage, and format:check. Final summary: 1118 tests, 1117 pass, 1 skipped; coverage 100% lines, branches, and functions for the covered report.
  • codex review -c service_tier='fast' --uncommitted passed with no actionable correctness issues. The review also reran pnpm run build:repair, node --test test/repair/command-runner.test.ts, and a 116-case real .cmd argument probe with 0 mismatches.

Redacted Windows terminal excerpt for the focused batch launcher path:

> node --test test\repair\command-runner.test.ts
? runCommand handles validation output larger than Node's sync spawn default (69.8391ms)
? runCommand reports command timeouts with the rendered command (26.8365ms)
? runCommand double-escapes Windows batch launcher arguments (2.9253ms)
? runCommand preserves Windows batch launcher arguments at runtime (99.3071ms)
? tests 4
? suites 0
? pass 4
? fail 0
? cancelled 0
? skipped 0
? todo 0
? duration_ms 341.6374

Additional .cmd edge-case probe after the fix:

{
  "equal": true,
  "args": [
    "quote\"x",
    "trailing\\",
    "double\\\\",
    "space trailing\\",
    "a b\\",
    "a b\\\"",
    "caret^x",
    "percent%PATH%",
    "brack]"
  ]
}

Full check closeout:

pnpm run check
...
? tests 1118
? pass 1117
? fail 0
? skipped 1
? coverage: 100.00% lines, 100.00% branches, 100.00% functions
All matched files use the correct format.
PNPM_CHECK_EXIT=0

@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 4:45 PM ET / 20:45 UTC.

Summary
The PR centralizes command-bin overrides and Windows launcher handling for Codex, repair, and GitHub CLI paths, normalizes Windows-sensitive state publishing and source reads, and updates repair/unit fixtures for Windows portability.

Reproducibility: no. high-confidence runtime reproduction was performed in this read-only review. Source inspection shows current main directly spawns repair commands and uses Windows-sensitive assumptions, while the PR discussion and tests identify the Windows failure mode.

Review metrics: 3 noteworthy metrics.

  • Changed surface: 25 files, +617/-329. The PR is not only fixture cleanup; it touches runtime command helpers, repair helpers, state publishing, and broad tests.
  • Runtime helpers touched: 7 source files changed. Command resolution, Codex spawning, GitHub CLI routing, target fanout, and state publish behavior all need maintainer attention before merge.
  • Proofed tree drift: 3 later non-empty commits after 3f76fb9. The PR body’s Windows terminal proof predates the latest shared resolver and case-insensitive lookup changes.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦐 gold shrimp
Patch quality: 🐚 platinum hermit
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Update the PR body with redacted current-head Windows terminal proof for b8444be or later, including the focused repair test and .cmd argument round-trip path.
  • [P1] Wait for the aggregate pnpm check on the latest head to finish green before merge.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR body includes terminal Windows proof for 3f76fb9, but current head b8444be has three later non-empty resolver commits; current-head proof is still needed, and private details should be redacted before posting. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The PR changes command execution and GitHub CLI shim routing used by ClawSweeper automation, so a subtle launcher regression could affect repair, review, or fanout workflows.
  • [P1] The Windows .cmd/.bat path crosses a shell boundary through cmd.exe with custom quoting, which needs current-head runtime proof rather than relying only on source inspection and simulated tests.
  • [P1] The PR body’s real Windows proof predates the latest shared resolver commits; CI is useful supplemental evidence, but it is not the contributor-facing real behavior proof gate by itself.
  • [P1] The aggregate pnpm check was still pending at review time, so maintainers should wait for the current-head check result before merging.

Maintainer options:

  1. Refresh current-head Windows proof (recommended)
    Add redacted terminal proof from b8444be or later that shows the Windows launcher path, focused repair test, and full check result after the shared resolver commits.
  2. Accept CI as maintainer-owned proof
    Maintainers could decide the passing focused Windows launcher check plus final green pnpm check is enough, but that would explicitly waive the external PR real-behavior proof gap.
  3. Pause if launcher policy is unsettled
    If maintainers are not ready to bless shared cmd.exe launcher handling, keep the PR open while narrowing or documenting the supported command-launch contract.

Next step before merge

  • [P1] Manual follow-up is needed because the remaining action is current-head proof and check validation, not a narrow code repair ClawSweeper can safely produce.

Security
Cleared: No concrete security or supply-chain vulnerability was found; the remaining shell-boundary concern is tracked as merge risk and proof requirement.

Review details

Best possible solution:

Land the shared Windows command resolver after current-head Windows terminal proof is added and the aggregate checks finish green.

Do we have a high-confidence way to reproduce the issue?

No high-confidence runtime reproduction was performed in this read-only review. Source inspection shows current main directly spawns repair commands and uses Windows-sensitive assumptions, while the PR discussion and tests identify the Windows failure mode.

Is this the best way to solve the issue?

Yes, the shared resolver and targeted fixture normalization are a narrow maintainable path for the reported Windows portability issue. The remaining gap is current-head runtime proof and final check completion, not a different code direction.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 44f0d7ac1222.

Label changes

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • remove rating: 🧂 unranked krab: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.

Label justifications:

  • P2: This is a normal-priority Windows portability and automation reliability fix with limited blast radius but real merge sensitivity.
  • merge-risk: 🚨 automation: The diff changes command execution, GitHub CLI shim routing, repair validation, and fanout helper paths used by ClawSweeper automation.
  • merge-risk: 🚨 security-boundary: The diff routes Windows batch launchers through cmd.exe with custom argument quoting, which is shell-boundary sensitive even without a concrete vulnerability finding.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body includes terminal Windows proof for 3f76fb9, but current head b8444be has three later non-empty resolver commits; current-head proof is still needed, and private details should be redacted before posting. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its conservative automation-safety and repair-lane guidance applies because this PR changes command execution, GitHub CLI helpers, state publishing, and repair tests. (AGENTS.md:1, 44f0d7ac1222)
  • Current main lacks the central repair runner change: On current main, repair validation still calls spawnSync(command, commandArgs) directly, so the central Windows launcher handling is not implemented on main. (src/repair/command-runner.ts:18, 44f0d7ac1222)
  • Latest PR head shares Windows spawn resolution: At PR head b8444be, resolveSpawnCommand handles *_BIN overrides, Windows PATH/PATHEXT lookup, node shebang scripts, .exe/.com launchers, and .cmd/.bat launchers through cmd.exe with windowsVerbatimArguments. (src/command.ts:101, b8444bef4720)
  • Previous lookup blocker was addressed in the latest head: The latest head now checks actualCasePath before the exact-case existsSync fallback, matching the prior failure around .CMD PATHEXT and lower-case codex.cmd fixtures. (src/command.ts:172, b8444bef4720)
  • Focused tests cover the Windows launcher path: The PR head includes simulated Windows coverage for Codex PATH/PATHEXT lookup and command-runner batch argument escaping. (test/codex-process.test.ts:24, b8444bef4720)
  • Proof drift from current head: The PR body says the real Windows proof was for 3f76fb9, but the current head is b8444be with later non-empty commits 270af59, b4771ea, and b8444be changing shared command-resolution code. (b8444bef4720)

Likely related people:

  • RomneyDa: Current main blame ties the Codex spawn, repair command runner, and state publish helpers to d9d50ab, and the latest PR commits refactor shared command resolution and fix Windows case-insensitive lookup. (role: introduced behavior and recent branch refactor author; confidence: high; commits: d9d50ab6f70d, 270af59fe1b4, b4771eab9ae9; files: src/codex-spawn.ts, src/command.ts, src/repair/command-runner.ts)
  • brokemac79: Prior merged local-review work touched command and Codex process helpers, and the first commits in this PR introduce the Windows repair-test portability changes. (role: adjacent command contributor; confidence: medium; commits: 79e768844291, ca6ff16fd56f, c9b398bd2071; files: src/command.ts, src/codex-spawn.ts, test/command.test.ts)
  • Vincent Koc: Recent merged CI/API-pressure work touched adjacent GitHub retry and CLI helper paths that this PR routes through the shared command override helper. (role: recent GitHub CLI helper contributor; confidence: medium; commits: 1566ad6a0a5b; files: src/repair/github-cli.ts, src/clawsweeper.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Jun 27, 2026
@brokemac79
brokemac79 force-pushed the codex/windows-test-portability branch from a183094 to ca6ff16 Compare June 27, 2026 18:58
@brokemac79

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 27, 2026
@brokemac79
brokemac79 force-pushed the codex/windows-test-portability branch from ca2f607 to 3f76fb9 Compare June 27, 2026 19:38
@brokemac79

Copy link
Copy Markdown
Contributor Author

@clawsweeper review

@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 30, 2026
@RomneyDa

Copy link
Copy Markdown
Member

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 30, 2026
@RomneyDa

RomneyDa commented Jul 1, 2026

Copy link
Copy Markdown
Member

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@RomneyDa
RomneyDa merged commit 03a13e0 into openclaw:main Jul 1, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants